Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restrict Custom Homebrew Item Properties #3

Merged
merged 12 commits into from
Sep 18, 2023
Merged

Restrict Custom Homebrew Item Properties #3

merged 12 commits into from
Sep 18, 2023

Conversation

revilowaldow
Copy link
Contributor

@revilowaldow revilowaldow commented Sep 10, 2023 •
edited
Loading

This PR restricts additional item properties for homebrew to a dedicated homebrew object.

This has the following benefits:

  • Users will no longer be able to make key name typos in e.g. capitalisation and still pass site tests with data that does not work as expected.
  • Non-functional data on items will be removed and functional but extended data will be moved to a dedicated area
  • There should be no name conflicts if the main site chooses to add a new property but a homebrew was already extending using the same key name
  • It is easy to find the custom properties being used in a specific brew as they are kept together
  • In summary, the experience of writing and debugging brew becomes easier and less error prone.

This has the following disadvantages:

  • The reference syntax {{item.<key>}} becomes strictly worse for homebrew as {{item.homebrew.<key>}}
  • Aspects of how magicvariant includes/excludes works may need to be confirmed and reworked to work with this next level down object.
  • Summary, there may need to be some site work to enable this.

Fixes to homebrew data are being made here TheGiddyLimit/homebrew#1841
It is likely this will also serve as the PR for the migration of custom props to the new area

@revilowaldow revilowaldow marked this pull request as ready for review September 10, 2023 09:19
@TheGiddyLimit
Copy link
Owner

a fine plan, but not sure about calling it homebrew -- a bit misleading if e.g. it were used in prerelease content

Would be happy with customProperties instead (already used in deities.json, but avoid refactoring to a shared schema for now, as they won't line up)

@revilowaldow
Copy link
Contributor Author

Sounds good to me 👍

e.g. it were used in prerelease content

Would you like this formatted in the modes style below and made available to both UA and brew?

"$$if": {
	"modes": ["brew", "ua"],
	"value": {
		"customProperties": {
			// etc
		}
	}
},

@TheGiddyLimit
Copy link
Owner

might as well 👍

@revilowaldow revilowaldow marked this pull request as draft September 17, 2023 15:16
@revilowaldow revilowaldow marked this pull request as ready for review September 17, 2023 20:59
@TheGiddyLimit TheGiddyLimit merged commit 061569e into TheGiddyLimit:master Sep 18, 2023
@revilowaldow revilowaldow deleted the brewproperties branch September 18, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheGiddyLimit TheGiddyLimit TheGiddyLimit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@revilowaldow @TheGiddyLimit